Document commit SHA support for GitDagBundle tracking_ref - #69735
Conversation
99bc2f2 to
6af6a6a
Compare
|
@potiuk this is ready for review, CI is fully green now! One piece of feedback on the triage flow: the maintainer note was added directly into the PR description rather than as a comment, so I didn't get a GitHub notification and didn't realize there was action needed on my end until I checked back in. Posting maintainer triage notes as a regular PR comment (with a tag) would make sure they surface in notifications/email inbox. Thanks for the help getting this triaged! |
GitDagBundle.tracking_ref already works with a full commit SHA in practice (checkout, promote, and rollback), but the provider docs only described it as a branch or tag. Clarify the docstring and provider docs, add a commit-SHA example, note static-ref refresh behavior, and add test coverage for SHA-pinned tracking_ref. Co-Authored-By: Otto <noreply@astronomer.io>
Unlike moving a branch or tag ref, repointing tracking_ref to a new commit SHA is a dag_bundle_config_list config change, so it only takes effect after the Dag processor and workers are restarted to reload the configuration. Co-Authored-By: Otto <noreply@astronomer.io>
Clarify that promoting/rolling back a SHA-pinned tracking_ref (an environment-level config change) is a different mechanism from Dag bundle versioning, which lets individual runs be rerun with their original commit regardless of the bundle's current tracking_ref. Co-Authored-By: Otto <noreply@astronomer.io>
The promote/rollback test already exercises basic SHA checkout as its first step, so the standalone test was redundant. Also tighten the docs note down to two paragraphs without dropping any of the facts. Co-Authored-By: Otto <noreply@astronomer.io>
The rerun_with_latest_version distinction is a separate feature and isn't needed to document tracking_ref's SHA support; keep the note focused on the restart caveat. Co-Authored-By: Otto <noreply@astronomer.io>
…otion The doc previously said promoting or rolling back a SHA-pinned tracking_ref requires restarting both the Dag processor and workers, but only the Dag processor needs to be restarted to pick up the change. Co-Authored-By: Otto <noreply@astronomer.io>
…mitation Kaxil found that a Dag processor restart alone doesn't promote a SHA-pinned tracking_ref if the bundle's local storage survives the restart (the default): the working clone only fetches new objects during refresh(), which runs after the checkout in _initialize(), so a genuinely new SHA fails to check out until that storage is cleared. He also flagged that with disable_bundle_versioning, workers resolve code from their own tracking_ref rather than a recorded bundle version, so they need the updated config too, and that the existing promote/rollback test created its "new" commit before the objects were ever missing locally, so it never exercised the failure. Narrow the docs to describe rollback as reliable and promotion as needing cleared storage, split the test into a rollback case, a promote-without-clearing case that documents the current failure, and a promote-with-fresh-storage case, and link the underlying limitation from both the code and the test to apache#71388. Co-Authored-By: Otto <noreply@astronomer.io>
6af6a6a to
acebdbd
Compare
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
LGTM. Confirming the three points from the July review all landed, since they were raised by someone else and it should be on record that they were checked rather than assumed:
- Promotion vs. restart — the mechanism is real, and you didn't take it on faith:
_initializechecks out before the working clone has fetched, so a commit created after that clone was populated isn't there yet. The docs note now draws the distinction correctly (rollback reliable, promotion needs storage cleared), and the limitation is tracked in #71388 with the full URL both in the docs and as a comment at the code site — which is the convention for deferred work, so the next person readinggit.pyfinds the follow-up without leaving the file. - Dag processor vs. workers — restored, and scoped properly: with
[dag_processor] disable_bundle_versioningor the per-Dag parameter, workers resolve from their owntracking_refand do need the updated configuration. - The test that didn't exercise a real promote — now three tests that each mean something distinct.
One thing I want to be explicit about rather than leave as a silent pass: the middle test asserts the GitCommandError that promotion raises today. Pinning current broken behaviour in a test is usually something I'd push back on, but here it is the right call and worth keeping — it is labelled as a known limitation, tied to #71388, and carries instructions to flip it to asserting success once the fix lands. That turns the limitation into something CI will tell us about when it changes, instead of a paragraph in the docs that quietly goes stale.
The git-version alternation in match="reference is not a tree|unable to read tree" is fine — it is deliberate and explained.
This review was drafted by an AI-assisted tool and
confirmed by an Airflow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
4 participants
Clarifies that
GitDagBundle.tracking_refsupports a branch, tag, or full commit SHA — it already works with a SHA in practice, but the provider docs only described branch/tag. Adds a commit-SHA example to the bundle docs, a note on static-ref refresh behavior (a SHA-pinned bundle doesn't pick up new commits viarefresh_interval; promoting/rolling back requires updatingtracking_refin the bundle config), and test coverage for SHA checkout, promotion, and rollback.Verified manually against a live repository, and confirmed during review: rolling back a SHA-pinned
tracking_refto an already-local commit works reliably after a Dag processor restart. Promoting to a genuinely new SHA can fail on restart if the bundle's local storage survives it (the default) —_initialize()checks out the new ref before the working clone has fetched it, so the checkout fails until that storage is cleared (fresh pod, or deleted bundle directory). That limitation is tracked at #71388; the docs and tests here describe the current behavior accordingly rather than promising promotion just works with a restart. Also scoped the restart note to mention that withdisable_bundle_versioning, workers resolve code from their owntracking_refand need the updated config too, not just the Dag processor.Was generative AI tooling used to co-author this PR?
Generated-by: Otto following the guidelines
🤖 Generated by Otto
Important
🛠️ Maintainer triage note for @coleheflin · by
@potiuk· 2026-07-28 16:10 UTCHelpful heads-up from the maintainers — please address before this PR can be reviewed:
The ball is in your court — you've been assigned to this PR. Fix the above, then mark it Ready for review.
See the Pull Request quality criteria for how to fix each item. There is no rush.
Note: your branch is 345 commits behind
main— please rebase and push again to get up-to-date CI results.Automated triage — may be imperfect; a maintainer takes the next look. We use this two-stage triage process so maintainers' limited time goes to the conversation with you.